Add support for asynchronous retrieval from `RedisCache` #2717

jxblum · 2023-09-26T00:16:05Z

mp911de

I did a first review pass. We need to come up with a design that doesn't introduce reactive types for all of our users as that will introduce a dependency regression.

mp911de · 2023-09-27T06:43:51Z

src/main/java/org/springframework/data/redis/cache/BatchStrategies.java

@@ -68,6 +65,10 @@ public static BatchStrategy scan(int batchSize) {
 		return new Scan(batchSize);
 	}

+	private BatchStrategies() {


Our source files have the following order:

constructors

(private) methods called from constructors

static factory methods

Please revert this change.

Understood. Out of curiosity, where is this documented?

https://github.com/spring-projects/spring-framework/wiki/Code-Style#java-source-file-organization, as we didn't define anything beyond https://github.com/spring-projects/spring-data-build/blob/main/CONTRIBUTING.adoc#source-code-style

I noticed that we are not honoring several of these Code Style guidelines:

Import order (e.g. Java/Javax/Jakarta library imports first!)

Location of getter/setters for fields.

No line break after closing } and else / catch blocks.

Our Javadoc formatting is not consistent.

Etc.

mp911de · 2023-09-27T06:46:11Z

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

@@ -149,6 +137,103 @@ public byte[] get(String name, byte[] key, @Nullable Duration ttl) {
 		return result;
 	}

+	@Override


Introducing Mono introduces a hard dependency to Project Reactor even for non-reactive users that want to use the Jedis driver. We need a different design. Either delegation or a subclass could work.

Alternatively, if we stay on CompletableFuture, then this would move the problem to a different place.

I need to think on this further, but my first reaction (no pun intended) is anyone of the approaches: delegation, subclassing or simply aligning with CompletableFuture are all good.

I like starting from the perspective of CompletableFuture and working backwards.

I will revise this.

mp911de · 2023-09-27T06:46:58Z

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

+	}
+
+	@Nullable
+	private static byte[] nullSafeGetBytes(@Nullable ByteBuffer value) {


Nit: private methods go to the end of a source file.

mp911de · 2023-09-27T06:50:08Z

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

+			ByteBuffer wrappedKey = ByteBuffer.wrap(key);
+
+			// Do the same lock check as the regular Cache.get(key); be careful of blocking!
+			Mono<ByteBuffer> cacheLockCheckMono = Mono.fromFuture(CompletableFuture.supplyAsync(() -> {


This isn't ideal to allocate and block a default thread from the ForkJoin pool as we introduce a dependency to the ForkJoin thread pool relying on a different thread availability model.

While loops are tricky with Reactor. Using Flux.generate and (take/skip)(Until/While) would allow to build a loop based on a number of retries or a total wait duration to check whether the lock is set and proceed from there.

My knowledge of Reactor is still limited in regards to the proper way to do things.

Effectively, I want the Mono to wait for the cache lock to be released (if the cache by name was in fact locked in the first place) before attempting to do the caching operation (i.e. Redis get[Ex](..) command). So, I wouldn't really say it is looping so much as it is waiting, even though inside checkAndPotentiallyWaitUntilUnlocked(..) the method loops waiting for the lock to be release. Still, from the caller's perspective, it is just perceived as a "blocking" method.

I wasn't sure how to add an additional operation (i.e. checking the cache lock) to the Mono returned from Lettuce before invoking the Redis.getExcommand in a asynchronous, non-blocking way. I triedMono.doFirst(:Runnable)` but that blocks before subscribing.

I may need some extra guidance here.

Probably this needs some refinement, but as a starting point I would suggest:

Mono<Boolean> isLocked = …; isLocked.flatMap(hasLock -> { if(!hasLock){ return Mono.empty(); } return Flux.interval(sleepTime).takeUntilOther(isLocked).then(); });

I think I figured out what I need to do with this. In a nutshell, I used Flux.interface(delay, period).takeUntil(..). The tests passed as I expected them to.

mp911de · 2023-09-27T06:52:27Z

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

+	// Execution Strategy applied when Jedis (non-Reactive driver) is used.
+	// Treats API consistently (that is, using Reactive types, such as Mono) at the RedisCacheWriter level
+	// whether "technically Reactive" or not; clearly Jedis is not "Reactive".
+	private BiFunction<byte[], Duration, Mono<byte[]>> asyncExecutionStrategy(String cacheName) {


We should revisit this design approach. We previously avoided simulating reactive behavior when using blocking-only drivers across the portfolio. Ideally, we simply do not support Jedis for asynchronous cache operations. Otherwise, we open up potential for discussions leading to an increased ask to provide reactive operations on top of Jedis.

2 things.

First, my initial reaction was how to support our Jedis users, too. So this was the design I came up with.

Second, Spring Framework's contract for the Cache.retrieve(..) operation (e.g. Javadoc) is very clear... it must not block.

In fact, the approach I took (using CompletableFuture.supplyAsync(..) for Jedis) is precisely how Spring Framework handles asynchronous, non-blocking behavior in Cache.retrieve(..) for caching providers that do not support asynchronous, non-blocking operations, such as ConcurrentMapCache (source). In contrast, if the underlying caching provider, such as Caffeine, supports asynchronous, non-blocking operations (in other words, return types, such as CompletableFuture), then the implementation is trivial (source).

So, in conclusion, I would say.

Either, we support Jedis users by wrapping the simple cache op in an asynchronous manner, not unlike ConcurrentMapCache, or we do not support Jedis at all.

It would be wrong to support Jedis, but only in a blocking manner, which would violate the contract of Cache.retrieve(..).

I am leaning towards supporting Jedis users where possible and the implementation is rather simple.

Either, we support Jedis users by wrapping the simple cache op in an asynchronous manner, not unlike ConcurrentMapCache, or we do not support Jedis at all.

We decided against pretending our blocking infrastructure would support asynchronicity to avoid raising false expectations. RestTemplate, JPA, JdbcTemplate are several examples that have followed this pattern.

It is not wise to do things just because we can.

It would be wrong to support Jedis, but only in a blocking manner, which would violate the contract of Cache.retrieve(..).

retrieve is allowed to throw UnsupportedOperationException. The JCache Cache doesn't support async operations.

It is not wise to do things just because we can.

No need to state the obvious. Also, I was not part of the original/initial "decision" so I am largely unaware and unfamiliar with what the intended direction is for Spring Data Redis. I am simply acting in the best interest of our community, and I am simply stating my case in favor of this approach, not because we "can".

Also, many caching providers don't support async caching operations yet. Neither did the ConcurrentMapCache implementation in the core Spring Framework, but support was added for async caching behavior none-the-less.

The JCache library and overall API is an SPI (a specification, hence JSR-107), not a implementation or caching provider. This is not unlike Spring Framework's Cache Abstraction itself. As such, it is necessarily the lowest common denominator across an ecosystem of caching providers. By way of example, Caffeine, which provides async caching support, is also a JCache caching provider implementation.

Anyway, I happy to go in whatever direction we decide together.

mp911de · 2023-09-27T06:54:33Z

src/main/java/org/springframework/data/redis/cache/RedisCache.java

 		return value != null ? value : isAllowNullValues() ? NullValue.INSTANCE : null;
 	}

+	@Override
+	public CompletableFuture<?> retrieve(Object key) {
+


We should check whether the underlying RedisCacheWriter is even capable of asynchronous cache operations. If not, then we can fall back to super.retrieve.

Well, currently, true async is only supported if Lettuce is on the classpath and used as the Redis driver. If Jedis is used, then it requires help, as I designed and commented to above.

mp911de · 2023-09-27T06:55:28Z

src/main/java/org/springframework/data/redis/cache/RedisCacheWriter.java

+	 * @see java.nio.ByteBuffer
+	 * @since 3.2.0
+	 */
+	default Mono<byte[]> retrieve(String name, byte[] key) {


As mentioned above, declaring Mono introduces a hard dependency on Project Reactor for all users.

In-progress on changing this API design.

mp911de · 2023-09-27T06:56:35Z

src/main/java/org/springframework/data/redis/cache/RedisCacheWriter.java

+	 * @return the {@link Mono value} to which the {@link RedisCache} maps the given {@link byte[] key}.
+	 * @throws IllegalStateException if the Redis connection factory is not reactive.
+	 * @see reactor.core.publisher.Mono
+	 * @see java.nio.ByteBuffer


Nit: Why a reference to ByteBuffer if it isn't used? Also why @see Mono? Mono is already linked as return type and doesn't require redundant documentation.

Sorry, leftover documentation. Initially, I was using ByteBuffers for keys and then changed the RedisCacheWriter.retrieve(..) method signatures to align with the other RedisCacheWriter methods and operations (i.e. using a byte[] for keys).

My mistake.

mp911de · 2023-09-27T06:57:18Z

src/main/java/org/springframework/data/redis/util/ByteUtils.java

@@ -251,13 +252,10 @@ public static ByteBuffer getByteBuffer(String theString, Charset charset) {
 	 * @param buffer must not be {@literal null}.
 	 * @return the extracted bytes.
 	 * @since 2.1
+	 * @deprecated Use {@link #getBytes(ByteBuffer)} instead.


Nit: We should include the version since when this has been deprecated. @Deprecated has also a since attribute.

mp911de · 2023-09-27T07:00:21Z

src/test/java/org/springframework/data/redis/cache/DefaultRedisCacheWriterUnitTests.java

+		assertThat(value).isNotNull();
+		assertThat(new String(value)).isEqualTo("test");
+
+		verify(this.mockReactiveConnectionFactory, times(1)).getReactiveConnection();


I wonder how maintainable this will remain when we have almost 15 verifications and what net value of the test remains.

Generally looking at the new tests, there's 90% infrastructure and only 10% of actual test code left.

jxblum · 2023-10-04T04:08:08Z

@mp911de & @christophstrobl - I have completely reworked this PR and removed any strict dependency on Project Reactor. The API is simply tied to CompletableFuture now.

However, I left the option to support Jedis in the mix.

As an alternative, I added 1 more [OPTIONAL] commit demonstrating asynchronous caching behavior WITHOUT Jedis in the mix (i.e. only available when Lettuce is used), throwing the expected UnsupportedOperationException by delegating up to Cache.retrieve(..) if a non-reactive Redis driver is in use, such as Jedis.

I urge us to consider support for Jedis. But, if we decide not to support Jedis, then we simply need to merge the [OPTIONAL] commit and answer/remove the TODOs I placed in the codebase to make the questions/changes easier to find.

So, in summary, this PR is ready to merge once we decide which direction to go (with Jedis support or not).

…ucture. * Apply Java 17 syntax try-with-resources in DefaultRedisCacheWriter execute methods. * Organize source code * Edit Javadoc.

Refactors extractBytes(:ByteBuffer) to call getBytes(:ByteBuffer), thereby avoid the NullPointerException by throwing the more appropriate IllegalArgumentException with a descriptive message instead.

Closes #2650

Replace blocking lock wait with non-blocking flow. Add support for asynchronous storage to persist the cache value after retrieval from the value supplier. Introduce AsyncCacheWriter abstraction to improve functional guards. Reformat code. Remove redundant tests. Revisit deprecation notices with consistent mention of the version in which the deprecation was introduced. Refine exception messages when RedisCache does not support async retrieval.

We now enable tests without an instance in EnabledOnRedisDriverCondition assuming that we only use DriverQualifier on instance fields and not static ones.

jxblum · 2023-10-11T02:13:17Z

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

@@ -59,6 +60,9 @@
 */
 class DefaultRedisCacheWriter implements RedisCacheWriter {

+	private static final boolean REACTIVE_REDIS_CONNECTION_FACTORY_PRESENT = ClassUtils
+			.isPresent("org.springframework.data.redis.connection.ReactiveRedisConnectionFactory", null);


This is very unnatural and ugly line break. It read nicer if ClassUtils was on the next line, as follows:...

ClassUtils.isPresent(..)

jxblum · 2023-10-11T02:15:55Z

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

-			String message = String.format("Interrupted while waiting to unlock cache %s", name);
-
-			throw new PessimisticLockingFailureException(message, cause);
+			throw new PessimisticLockingFailureException(String.format("Interrupted while waiting to unlock cache %s", name),


Now you have introduced yet another unnatural line break between the formatted exception message and cause.

jxblum · 2023-10-11T02:18:08Z

src/main/java/org/springframework/data/redis/cache/RedisCache.java

-	 * @param cacheConfiguration {@link RedisCacheConfiguration} applied to this {@link RedisCache} on creation;
-	 * must not be {@literal null}.
+	 * @param cacheWriter {@link RedisCacheWriter} used to perform {@link RedisCache} operations by executing the
+	 *          necessary Redis commands; must not be {@literal null}.


These Javadoc indentations are now inconsistent with the core Spring Framework Javadoc foramatting guidelines.

jxblum · 2023-10-11T02:19:35Z

src/main/java/org/springframework/data/redis/cache/RedisCache.java

@@ -418,13 +429,23 @@ protected String convertKey(Object key) {
 			return key.toString();
 		}

-		String message = String.format("Cannot convert cache key %s to String; Please register a suitable Converter"
-				+ " via 'RedisCacheConfiguration.configureKeyConverters(...)' or override '%s.toString()'",
+		String message = String.format(


Another unnatural line break.

jxblum · 2023-10-11T02:21:02Z

src/main/java/org/springframework/data/redis/cache/RedisCacheManager.java

@@ -34,21 +34,20 @@
 /**
 * {@link CacheManager} implementation for Redis backed by {@link RedisCache}.
 * <p>
- * This {@link CacheManager} creates {@link Cache caches} on first write, by default. Empty {@link Cache caches}
- * are not visible in Redis due to how Redis represents empty data structures.
+ * This {@link CacheManager} creates {@link Cache caches} on first write, by default. Empty {@link Cache caches} are not


This is not a natural appropriate place grammatically to create lines breaks in (source) Javadoc, regardless of how it is rendered (HTML, PDF, or otherwise).

I spent quite a bit of effort to clean up these sort of Javadoc issues already.

Javadoc formatting is automated, so we should not spend too much effort in manually aligning these as the formatter is going to apply its formatting anyways.

I don't agree with this statement. When I am working on, and maintaining source code, I am looking at the Javadoc in the source, checking for accuracy, necessary edits, completeness, etc. I am not looking to the rendered (e.g. HTML) form.

jxblum · 2023-10-11T02:22:03Z

src/test/java/org/springframework/data/redis/cache/RedisCacheTests.java

-import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
-import static org.assertj.core.api.Assumptions.assumeThat;
-import static org.awaitility.Awaitility.await;
+import static org.assertj.core.api.Assertions.*;


No star imports.

jxblum · 2023-10-11T02:22:24Z

src/test/java/org/springframework/data/redis/cache/RedisCacheTests.java

@@ -353,8 +346,9 @@ void computePrefixCreatesCacheKeyCorrectly() {

 		cacheWithCustomPrefix.put("key-1", sample);

-		doWithConnection(connection -> assertThat(connection.stringCommands()
-				.get("_cache_key-1".getBytes(StandardCharsets.UTF_8))).isEqualTo(binarySample));
+		doWithConnection(


Ugly line break... occurring all the way down in this source file.

jxblum · 2023-10-11T02:24:27Z

src/test/java/org/springframework/data/redis/cache/RedisCacheUnitTests.java

 		verifyNoMoreInteractions(mockCacheWriter);
 	}

-	@Test // GH-2650


What happened to these test case methods?

These are testing internals (to some extent similar to testing getters and setters) and we have already tests for storing null values and null value caching in RedisCacheTests so it doesn't make sense to have redundancy causing additional maintenance efforts.

RedisCacheTests are "Integration" Tests whereas Unit Testing is a form of White-box testing. These tests are ensuring the correct handling of null values regardless of infrastructure concerns and are not equivalent to getter and setter testing.

I agree with removing redundancy.

jxblum · 2023-10-11T03:10:28Z

These changes need further, closer review inside my IDE as they are far too extensive to review in GitHub alone.

Cleanup ambigious, unreadable, unstructured and overly complex code in DefaultRedisCacheWriter. Split RedisCacheTests.retrieveCacheValueUsingJedis() test logic by test case. Edit Javadoc. See spring-projects#2650 Original pull request: spring-projects#2717

Cleanup ambigious, unreadable, unstructured and complex logic in DefaultRedisCacheWriter. Split RedisCacheTests.retrieveCacheValueUsingJedis() test logic by test case. Edit Javadoc. See spring-projects#2650 Original pull request: spring-projects#2717

Replace blocking lock wait with non-blocking flow. Add support for asynchronous storage to persist the cache value after retrieval from the value supplier. Introduce AsyncCacheWriter abstraction to improve functional guards. Reformat code. Remove redundant tests. Revisit deprecation notices with consistent mention of the version in which the deprecation was introduced. Refine exception messages when RedisCache does not support async retrieval. See spring-projects#2650 Original pull request: spring-projects#2717

Cleanup ambigious, unreadable, unstructured and complex logic in DefaultRedisCacheWriter. Split RedisCacheTests.retrieveCacheValueUsingJedis() test logic by test case. Edit Javadoc. See spring-projects#2650 Original pull request: spring-projects#2717

jxblum · 2023-10-12T23:30:36Z

This is now reviewed, polished and merged.

Closes #2650 Original pull request: #2717

Replace blocking lock wait with non-blocking flow. Add support for asynchronous storage to persist the cache value after retrieval from the value supplier. Introduce AsyncCacheWriter abstraction to improve functional guards. Reformat code. Remove redundant tests. Revisit deprecation notices with consistent mention of the version in which the deprecation was introduced. Refine exception messages when RedisCache does not support async retrieval. See #2650 Original pull request: #2717

We now enable tests without an instance in EnabledOnRedisDriverCondition assuming that we only use DriverQualifier on instance fields and not static ones. Closes #2734 Original pull request: #2717

Uses more descriptive names for operations, especially Reactive operations, by either calling a local, private method or introducing a variable with strongly typed parameters. Original Pull Request: spring-projects#2717 Closes spring-projects#2743

Uses more descriptive names for operations, especially Reactive operations, by either calling a local, private method or introducing a (functional) variable with strongly typed parameters. Original Pull Request: spring-projects#2717 Closes spring-projects#2743

Uses more descriptive names for operations, especially Reactive operations, by either calling a local, private method or introducing a (functional) variable with strongly typed parameters. Edits and refines Javadoc. Original Pull Request: spring-projects#2717 Closes spring-projects#2743

jxblum added in: cache RedisCache and CacheManager type: enhancement A general enhancement labels Sep 26, 2023

jxblum requested review from mp911de and christophstrobl September 26, 2023 19:11

mp911de requested changes Sep 27, 2023

View reviewed changes

jxblum force-pushed the issue/2650 branch from c1e0745 to 5315fa7 Compare October 4, 2023 01:27

jxblum added 4 commits October 10, 2023 14:07

Prepare issue branch for 2650.

8949892

Refactor code and cleanup compiler warnings in Redis caching infrastr…

a6f7416

…ucture. * Apply Java 17 syntax try-with-resources in DefaultRedisCacheWriter execute methods. * Organize source code * Edit Javadoc.

Deprecate redundant ByteUtils.extractBytes(…) method.

89876e2

Refactors extractBytes(:ByteBuffer) to call getBytes(:ByteBuffer), thereby avoid the NullPointerException by throwing the more appropriate IllegalArgumentException with a descriptive message instead.

Support asynchronous Cache.retrieve(…) in RedisCache.

3b62e12

Closes #2650

mp911de force-pushed the issue/2650 branch from 5315fa7 to b6f0fd9 Compare October 10, 2023 12:17

mp911de changed the title ~~Adds support for reactive types on retrieval from RedisCache~~ Adds support for asynchronous retrieval from RedisCache Oct 10, 2023

mp911de linked an issue Oct 10, 2023 that may be closed by this pull request

Add support for asynchronous retrieval from RedisCache #2650

Closed

mp911de force-pushed the issue/2650 branch from b6f0fd9 to 7c12717 Compare October 10, 2023 12:33

mp911de added 2 commits October 10, 2023 14:43

Allow @EnabledOnRedisDriver usage with @ParameterizedRedisTest.

2c638fd

We now enable tests without an instance in EnabledOnRedisDriverCondition assuming that we only use DriverQualifier on instance fields and not static ones.

mp911de force-pushed the issue/2650 branch from 7c12717 to 2c638fd Compare October 10, 2023 12:43

jxblum commented Oct 11, 2023

View reviewed changes

mp911de changed the title ~~Adds support for asynchronous retrieval from RedisCache~~ Add support for asynchronous retrieval from RedisCache Oct 12, 2023

jxblum closed this Oct 12, 2023

jxblum deleted the issue/2650 branch October 12, 2023 23:31

mp911de added this to the 3.2 RC1 (2023.1.0) milestone Oct 13, 2023

mp911de pushed a commit that referenced this pull request Oct 13, 2023

Support asynchronous Cache.retrieve(…) in RedisCache.

a72c426

Closes #2650 Original pull request: #2717

jxblum mentioned this pull request Oct 17, 2023

Refine and clarify operations in asynchronous caching implementation #2743

Open

Add support for asynchronous retrieval from RedisCache #2717

Add support for asynchronous retrieval from RedisCache #2717

Conversation

jxblum commented Sep 26, 2023

mp911de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxblum Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxblum Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxblum Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxblum commented Oct 4, 2023

jxblum Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

jxblum Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

jxblum Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxblum Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxblum Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxblum commented Oct 11, 2023

jxblum commented Oct 12, 2023

Add support for asynchronous retrieval from `RedisCache` #2717

Add support for asynchronous retrieval from `RedisCache` #2717

jxblum Sep 27, 2023 •

edited

Loading

jxblum Oct 3, 2023 •

edited

Loading

jxblum Sep 27, 2023 •

edited

Loading

jxblum Oct 11, 2023 •

edited

Loading

jxblum Oct 11, 2023 •

edited

Loading

jxblum Oct 11, 2023 •

edited

Loading

jxblum Oct 11, 2023 •

edited

Loading

jxblum Oct 11, 2023 •

edited

Loading